refactor(openai-bridge): split runtime retry layers#212
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds configurable, bounded OpenAI-bridge retries across provider configuration, persistence, API responses, runtime forwarding, and the web UI. Bridge conversion and SSE handling are extracted into a shared runtime module, with tests covering retry behavior and provider state. ChangesOpenAI bridge retry and runtime
Prompts tab presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAI Bridge
participant Retry Utility
participant Upstream API
Client->>OpenAI Bridge: Send bridge request
OpenAI Bridge->>Retry Utility: Execute with maxRetries
Retry Utility->>Upstream API: Forward request
Upstream API-->>Retry Utility: Response or transient failure
Retry Utility-->>OpenAI Bridge: Final upstream result
OpenAI Bridge-->>Client: Responses-compatible response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli.js (1)
12870-12886: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the already-resolved
upstream.maxRetriesinstead of re-deriving from config.toml.
upstream(fromresolveOpenaiBridgeUpstream) already carriesmaxRetriessourced fromOPENAI_BRIDGE_SETTINGS_FILE— the value actually used byretryTransientRequestat request time. Instead, this handler discards it and re-readsconfig.toml'scodexmate_bridge_max_retriesmirror viaresolveProviderOpenaiBridgeMaxRetries(provider). If the two stores ever drift (partial write failure, manual edit, pre-existing provider without the TOML field), the UI's edit form will be pre-filled with a stale value, and saving without touching this field will silently overwrite the real setting inOPENAI_BRIDGE_SETTINGS_FILEwith the stale one. The same pattern recurs inbuildMcpProviderListPayload(Lines 15116-15131), which also ignoresupstream.maxRetriesfor itsopenaiBridgeMaxRetriesfield.🐛 Suggested fix
- // 不返回 apiKey（敏感信息），仅返回用户填过的上游 URL - const config = readConfig(); - const provider = config.model_providers && config.model_providers[name]; - const providerMaxRetries = resolveProviderOpenaiBridgeMaxRetries(provider); - result = { baseUrl: upstream.baseUrl, hasApiKey: !!(upstream.apiKey), maxRetries: providerMaxRetries }; + // 不返回 apiKey（敏感信息），仅返回用户填过的上游 URL + result = { baseUrl: upstream.baseUrl, hasApiKey: !!(upstream.apiKey), maxRetries: upstream.maxRetries };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli.js` around lines 12870 - 12886, Use the already-resolved upstream.maxRetries when constructing the openai-bridge-get-provider result instead of re-reading config.toml through resolveProviderOpenaiBridgeMaxRetries. Also update buildMcpProviderListPayload to populate openaiBridgeMaxRetries from its resolved upstream.maxRetries, preserving the request-time settings as the source of truth.
🧹 Nitpick comments (2)
cli/openai-bridge-runtime.js (1)
1339-1797: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: deduplicate the HTTP request scaffolding.
streamChatCompletionsAsResponsesSse,streamResponsesSse, andproxyRequestJsonrepeat near-identical setup (URL parse, transport selection, header/Content-Lengthassembly,maxBytes/timeoutMsnormalization,finish/settledguard,abortUpstreamonresclose). Extracting a small helper for request construction and the settle/abort plumbing would reduce drift risk across the three paths (e.g., the timeout asymmetry noted above).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/openai-bridge-runtime.js` around lines 1339 - 1797, The HTTP request setup and settlement/abort plumbing is duplicated across streamChatCompletionsAsResponsesSse, streamResponsesSse, and proxyRequestJson. Extract a focused shared helper for URL/transport selection, headers and Content-Length, maxBytes/timeoutMs normalization, settled resolution, and response-close abortion, then update all three functions to use it while preserving their endpoint-specific streaming and error behavior.cli/openai-bridge.js (1)
167-200: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
maxRetrieshas no existing-value fallback, unlikeheaders.
headersfalls back toexistingHeaderswhen no new headers are supplied, butmaxRetriesis always taken fromnormalizeBridgeMaxRetries(options && options.maxRetries)with no fallback to the currently stored value. Any future caller that omitsoptions.maxRetrieswill silently reset a previously configured retry count back to the default. Today's callers incli.jswork around this by always resolving a value up front (including one path that re-derives it fromconfig.tomlviaresolveProviderOpenaiBridgeMaxRetries), but that's fragile — it would be safer to make this function preserve the existing value itself.♻️ Suggested fix
const existing = settings && settings.providers ? settings.providers[name] : null; const existingHeaders = existing && typeof existing === 'object' && !Array.isArray(existing) ? normalizeHeadersMap(existing.headers || existing.extraHeaders || existing.extra_headers || null) : {}; + const existingMaxRetries = existing && typeof existing === 'object' && !Array.isArray(existing) && existing.maxRetries !== undefined + ? existing.maxRetries + : undefined; + const maxRetries = normalizeBridgeMaxRetries( + options && options.maxRetries !== undefined ? options.maxRetries : existingMaxRetries + ); const next = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/openai-bridge.js` around lines 167 - 200, Update upsertOpenaiBridgeProvider to preserve the existing provider’s maxRetries when options.maxRetries is omitted, mirroring the existingHeaders fallback. Read and normalize the stored value from existing before selecting the new value, while continuing to use the normalized option when explicitly supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/openai-bridge-runtime.js`:
- Around line 1538-1541: The request timeout in
streamChatCompletionsAsResponsesSse remains active after the upstream response
switches to SSE. Clear or otherwise disable that timeout at the point SSE
acceptance is confirmed, matching streamResponsesSse, so idle streaming cannot
invoke finish with a timeout after partial output or emit duplicate terminal
responses.
In `@web-ui/modules/app.methods.providers.mjs`:
- Around line 89-97: Update the provider validation flow around
normalizeBridgeMaxRetries and the openaiBridgeMaxRetries check so validation
uses the raw, unclamped draft input first and sets errors.openaiBridgeMaxRetries
when the value is below 2; only normalize the value after validation for
submission. Preserve submit-time clamping and add a regression test covering the
inline minimum-retries error message.
In `@web-ui/modules/i18n/locales/zh-tw.mjs`:
- Around line 491-492: Update the hint.transformMaxRetries translation to
mention the setting’s maximum value of 10 alongside the existing default and
minimum values, while preserving the surrounding retry-count explanation.
In `@web-ui/modules/i18n/locales/zh.mjs`:
- Around line 491-492: Update the `hint.transformMaxRetries` translation in the
Chinese locale to mention the enforced maximum of 10 alongside the existing
default of 2 and minimum of 2; leave the `field.transformMaxRetries` label
unchanged.
---
Outside diff comments:
In `@cli.js`:
- Around line 12870-12886: Use the already-resolved upstream.maxRetries when
constructing the openai-bridge-get-provider result instead of re-reading
config.toml through resolveProviderOpenaiBridgeMaxRetries. Also update
buildMcpProviderListPayload to populate openaiBridgeMaxRetries from its resolved
upstream.maxRetries, preserving the request-time settings as the source of
truth.
---
Nitpick comments:
In `@cli/openai-bridge-runtime.js`:
- Around line 1339-1797: The HTTP request setup and settlement/abort plumbing is
duplicated across streamChatCompletionsAsResponsesSse, streamResponsesSse, and
proxyRequestJson. Extract a focused shared helper for URL/transport selection,
headers and Content-Length, maxBytes/timeoutMs normalization, settled
resolution, and response-close abortion, then update all three functions to use
it while preserving their endpoint-specific streaming and error behavior.
In `@cli/openai-bridge.js`:
- Around line 167-200: Update upsertOpenaiBridgeProvider to preserve the
existing provider’s maxRetries when options.maxRetries is omitted, mirroring the
existingHeaders fallback. Read and normalize the stored value from existing
before selecting the new value, while continuing to use the normalized option
when explicitly supplied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e26c54c-4d1a-4ef9-a74c-ac3bd4276b1f
📒 Files selected for processing (18)
cli.jscli/openai-bridge-retry.jscli/openai-bridge-runtime.jscli/openai-bridge.jstests/unit/openai-bridge-upstream-responses.test.mjstests/unit/provider-default-names.test.mjstests/unit/provider-share-command.test.mjstests/unit/provider-switch-regression.test.mjstests/unit/providers-validation.test.mjsweb-ui/app.jsweb-ui/modules/app.methods.providers.mjsweb-ui/modules/i18n/locales/en.mjsweb-ui/modules/i18n/locales/ja.mjsweb-ui/modules/i18n/locales/vi.mjsweb-ui/modules/i18n/locales/zh-tw.mjsweb-ui/modules/i18n/locales/zh.mjsweb-ui/partials/index/modals-basic.htmlweb-ui/res/web-ui-render.precompiled.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🧰 Additional context used
🪛 ast-grep (0.44.1)
cli/openai-bridge-retry.js
[warning] 37-37: Avoid using the initial state variable in setState
Context: setTimeout(r, delay)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
cli.js
[warning] 10040-10040: Detects non-literal values in regular expressions
Context: new RegExp(^(\\s*${escapedFieldName}\\s*=\\s*)([-+]?\\d+(?:\\.\\d+)?)(\\s+#.*)?$, 'mg')
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
cli/openai-bridge-runtime.js
[warning] 1647-1649: Avoid using the initial state variable in setState
Context: setTimeout(() => {
failAcceptedStream('upstream stream idle timeout');
}, timeoutMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (19)
cli/openai-bridge-retry.js (1)
1-62: LGTM!cli.js (3)
2565-2584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate clamp/default logic vs.
openai-bridge-runtime.js'snormalizeBridgeMaxRetries.This mirrors the concern raised in
cli/openai-bridge.js: two independent implementations of essentially the same "clamp retries, default 2" logic exist in different files/files, governing two different persisted copies of the same setting (config.tomlvsOPENAI_BRIDGE_SETTINGS_FILE). Consider sharing a single normalizer to guarantee they never diverge.
9873-9877: LGTM!cmdUpdate'sopenaiBridgeMaxRetriesoption handling and the newreplaceTomlNumberFieldhelper (mirroring the existing string/boolean field replacers) are wired consistently, and the two writes (settings.json + config.toml) happen together whenever the field is explicitly changed. The static-analysis "non-literal RegExp" hint onreplaceTomlNumberFieldis a false positive here —fieldNameis always a hardcoded literal ('codexmate_bridge_max_retries'), not user input.Also applies to: 10037-10062, 10097-10150
2586-2708: LGTM!addProviderToConfig/updateProviderInConfigconsistently threadopenaiBridgeMaxRetriesinto bothupsertOpenaiBridgeProviderand thecodexmate_bridge_max_retriesTOML line.Also applies to: 2710-2738
cli/openai-bridge.js (2)
378-582: LGTM! All fiveretryTransientRequestcall sites consistently forward{ maxRetries: upstream.maxRetries }.
89-108: 📐 Maintainability & Code QualityNo mismatch in max-retry normalizers
normalizeBridgeMaxRetriesuses the same default (2) and clamp ([2, 10]) asnormalizeOpenaiBridgeMaxRetries, so the UI/config and runtime paths stay aligned.> Likely an incorrect or invalid review comment.web-ui/app.js (1)
287-289: LGTM! DefaultopenaiBridgeMaxRetries: 2matches the backend'snormalizeOpenaiBridgeMaxRetriesdefault.tests/unit/openai-bridge-upstream-responses.test.mjs (1)
96-141: LGTM! Solid end-to-end coverage of provider-configuredmaxRetriesagainst a transient upstream failure, with proper teardown.tests/unit/provider-share-command.test.mjs (1)
1470-1472: LGTM! Test double updated to matchbuildMcpProviderListPayload's new dependency onresolveProviderOpenaiBridgeMaxRetries.tests/unit/provider-switch-regression.test.mjs (1)
273-273: LGTM! Matches the updated defaulteditingProvidershape inweb-ui/app.js.web-ui/modules/app.methods.providers.mjs (2)
44-49: LGTM!
151-243: LGTM!Also applies to: 337-365, 367-439, 441-501, 580-584
web-ui/partials/index/modals-basic.html (1)
56-68: LGTM!Also applies to: 107-119
web-ui/res/web-ui-render.precompiled.js (1)
7098-7128: LGTM!Also applies to: 7241-7271
web-ui/modules/i18n/locales/en.mjs (1)
491-492: LGTM!web-ui/modules/i18n/locales/ja.mjs (1)
493-494: LGTM!web-ui/modules/i18n/locales/vi.mjs (1)
711-712: LGTM!tests/unit/provider-default-names.test.mjs (1)
41-42: LGTM!tests/unit/providers-validation.test.mjs (1)
25-26: LGTM!Also applies to: 97-97, 183-213
| upstreamReq.setTimeout(timeoutMs, () => { | ||
| try { upstreamReq.destroy(new Error('timeout')); } catch (_) {} | ||
| finish({ ok: false, error: 'timeout' }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the streamed result (ok/status) is consumed after streamChatCompletionsAsResponsesSse
rg -nP -C8 'streamChatCompletionsAsResponsesSse' cli/openai-bridge.jsRepository: SakuraByteCore/codexmate
Length of output: 2415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cli/openai-bridge-runtime.js around streamChatCompletionsAsResponsesSse ---'
nl -ba cli/openai-bridge-runtime.js | sed -n '1460,1605p'
echo
echo '--- cli/openai-bridge-runtime.js around streamResponsesSse ---'
nl -ba cli/openai-bridge-runtime.js | sed -n '1605,1735p'
echo
echo '--- cli/openai-bridge.js around the stream call site ---'
nl -ba cli/openai-bridge.js | sed -n '450,530p'Repository: SakuraByteCore/codexmate
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cli/openai-bridge-runtime.js around streamChatCompletionsAsResponsesSse ---'
cat -n cli/openai-bridge-runtime.js | sed -n '1460,1605p'
echo
echo '--- cli/openai-bridge-runtime.js around streamResponsesSse ---'
cat -n cli/openai-bridge-runtime.js | sed -n '1605,1735p'
echo
echo '--- cli/openai-bridge.js around the stream call site ---'
cat -n cli/openai-bridge.js | sed -n '450,530p'Repository: SakuraByteCore/codexmate
Length of output: 20131
🏁 Script executed:
node - <<'JS'
const http = require('http');
async function test(clearAfterStart) {
return new Promise((resolve, reject) => {
const server = http.createServer((req, res) => {
res.writeHead(200, { 'Content-Type': 'text/event-stream' });
res.write('data: first\n\n');
if (clearAfterStart) {
// Simulate the accepted-stream behavior from streamResponsesSse.
req.setTimeout(0);
}
setTimeout(() => {
try { res.write('data: second\n\n'); } catch {}
try { res.end(); } catch {}
}, 250);
});
server.listen(0, '127.0.0.1', () => {
const { port } = server.address();
const req = http.request({
host: '127.0.0.1',
port,
path: '/',
method: 'GET'
}, (res) => {
res.on('data', () => {});
res.on('end', () => {
server.close(() => resolve({ clearAfterStart, timedOut }));
});
});
let timedOut = false;
req.setTimeout(100, () => {
timedOut = true;
try { req.destroy(new Error('timeout')); } catch {}
});
req.on('error', () => {});
req.end();
setTimeout(() => {
if (!server.listening) return;
server.close(() => resolve({ clearAfterStart, timedOut }));
}, 500);
});
});
}
(async () => {
console.log(await test(false));
console.log(await test(true));
})();
JSRepository: SakuraByteCore/codexmate
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- timeout-related references in cli/openai-bridge-runtime.js ---'
rg -n "setTimeout\\(|streamIdleTimer|writeSse\\(|response\\.failed|finish\\(\\{ ok: false" cli/openai-bridge-runtime.js
echo
echo '--- streamChatCompletionsAsResponsesSse full function context ---'
python3 - <<'PY'
from pathlib import Path
text = Path('cli/openai-bridge-runtime.js').read_text()
start = text.index('function streamChatCompletionsAsResponsesSse')
end = text.index('function streamResponsesSse')
for i, line in enumerate(text[start:end].splitlines(), 1):
print(f"{i:5d} {line}")
PY
echo
echo '--- retryTransientRequest implementation ---'
python3 - <<'PY'
from pathlib import Path
text = Path('cli/openai-bridge-runtime.js').read_text()
start = text.index('async function retryTransientRequest')
end = text.index('function isTransientNetworkError')
for i, line in enumerate(text[start:end].splitlines(), 1):
print(f"{i:5d} {line}")
PYRepository: SakuraByteCore/codexmate
Length of output: 15114
🌐 Web query:
Node.js ClientRequest.setTimeout response stream inactivity timeout mid-stream docs
💡 Result:
In Node.js, the ClientRequest.setTimeout() method behaves as an inactivity timer, not a total request duration timer [1][2]. It monitors the underlying socket for inactivity—meaning it triggers only if no data has been received or sent over the socket for the specified duration [1][3]. Key behaviors regarding mid-stream inactivity: - Inactivity Reset: The timeout timer is reset whenever new data is received [1][4]. If you are streaming a large response, the timer will not fire as long as data packets continue to arrive within the timeout interval, even if the total time taken to stream the response far exceeds the timeout value [2]. - Socket Dependency: The timer is only armed once a socket has been assigned to the request and successfully connected [1][5]. It does not account for the time spent in the DNS lookup or the initial TCP connection handshake [5][6]. - Manual Destruction: When the timeout event is emitted, Node.js notifies you that the socket has been idle, but it does not automatically abort or destroy the request [7][3]. You must explicitly call request.destroy() or request.abort() within the timeout handler to stop the operation [7][3]. To enforce a "total duration" timeout (i.e., a maximum time from request initiation to completion), you must implement a separate timer (using setTimeout) that runs independently of the socket's inactivity state [5]. This manual timer should be cleared when the request finishes or errors out [5][6].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/http.html
- 2: request.setTimeout should be used for inactivity follow-redirects/follow-redirects#181
- 3: https://github.com/nodejs/node/blob/159ae48f/doc/api/http.md
- 4: https://beta.docs.nodejs.org/http.html
- 5: https://stackoverflow.com/questions/79534776/why-can-clientrequest-settimeout-take-much-longer-than-the-provided-duration
- 6: fix(http): timeout not honored during TCP connect phase when maxRedirects=0 axios/axios#10818
- 7: https://nodejs.org/api/http.html
Clear the request timeout once SSE is accepted. In streamChatCompletionsAsResponsesSse, the request-level timeout stays armed for the whole stream, unlike streamResponsesSse which clears it after switching to SSE. An idle upstream stream can then resolve { ok: false, error: 'timeout' } after partial output has already been written, and cli/openai-bridge.js will emit a second response.failed/done on the same res.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/openai-bridge-runtime.js` around lines 1538 - 1541, The request timeout
in streamChatCompletionsAsResponsesSse remains active after the upstream
response switches to SSE. Clear or otherwise disable that timeout at the point
SSE acceptance is confirmed, matching streamResponsesSse, so idle streaming
cannot invoke finish with a timeout after partial output or emit duplicate
terminal responses.
| const useTransform = !!(draft && draft.useTransform); | ||
| const openaiBridgeMaxRetries = normalizeBridgeMaxRetries(draft && draft.openaiBridgeMaxRetries); | ||
| const errors = { | ||
| name: '', | ||
| url: '', | ||
| key: '', | ||
| model: '' | ||
| model: '', | ||
| openaiBridgeMaxRetries: '' | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Minimum-retries validation is unreachable dead code.
openaiBridgeMaxRetries is normalized via normalizeBridgeMaxRetries (Line 90) before the < 2 check (Line 127) runs, and that helper always clamps its output to at least 2. So openaiBridgeMaxRetries < 2 can never be true, and errors.openaiBridgeMaxRetries can never be set — the “重试次数最小为 2” message will never show, regardless of what the user types. This silently defeats the minimum-retries validation that the PR objectives explicitly call out ("Web UI screenshots for ... minimum validation").
The fix is to validate the raw (un-clamped) input before normalizing:
🐛 Proposed fix
const useTransform = !!(draft && draft.useTransform);
- const openaiBridgeMaxRetries = normalizeBridgeMaxRetries(draft && draft.openaiBridgeMaxRetries);
+ const rawMaxRetries = Number(draft && draft.openaiBridgeMaxRetries);
+ const openaiBridgeMaxRetries = normalizeBridgeMaxRetries(draft && draft.openaiBridgeMaxRetries);
const errors = {
name: '',
url: '',
key: '',
model: '',
openaiBridgeMaxRetries: ''
};
@@
- if (useTransform && openaiBridgeMaxRetries < 2) {
+ if (useTransform && Number.isFinite(rawMaxRetries) && rawMaxRetries < 2) {
errors.openaiBridgeMaxRetries = '重试次数最小为 2';
}Also worth adding a regression test for this once fixed (the new test in providers-validation.test.mjs only covers submit-time clamping, not the inline error message).
Also applies to: 127-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/app.methods.providers.mjs` around lines 89 - 97, Update the
provider validation flow around normalizeBridgeMaxRetries and the
openaiBridgeMaxRetries check so validation uses the raw, unclamped draft input
first and sets errors.openaiBridgeMaxRetries when the value is below 2; only
normalize the value after validation for submission. Preserve submit-time
clamping and add a regression test covering the inline minimum-retries error
message.
| 'field.transformMaxRetries': '內建轉換重試次數', | ||
| 'hint.transformMaxRetries': '預設 2，最小 2；表示失敗後最多重試的次數。', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hint text omits the max-10 boundary.
The PR adds a max UI boundary of 10 for this setting, but the hint only mentions the default/min ("預設 2，最小 2"). Consider mentioning the upper bound too, so users aren't confused when a higher value gets silently clamped.
✏️ Suggested wording
- 'hint.transformMaxRetries': '預設 2，最小 2；表示失敗後最多重試的次數。',
+ 'hint.transformMaxRetries': '預設 2，範圍 2-10；表示失敗後最多重試的次數。',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'field.transformMaxRetries': '內建轉換重試次數', | |
| 'hint.transformMaxRetries': '預設 2，最小 2；表示失敗後最多重試的次數。', | |
| 'field.transformMaxRetries': '內建轉換重試次數', | |
| 'hint.transformMaxRetries': '預設 2，範圍 2-10；表示失敗後最多重試的次數。', |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/i18n/locales/zh-tw.mjs` around lines 491 - 492, Update the
hint.transformMaxRetries translation to mention the setting’s maximum value of
10 alongside the existing default and minimum values, while preserving the
surrounding retry-count explanation.
| 'field.transformMaxRetries': '内建转换重试次数', | ||
| 'hint.transformMaxRetries': '默认 2，最小 2；表示失败后最多重试的次数。', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hint text omits the max-10 boundary.
Same issue as zh-tw.mjs: the hint only states "默认 2，最小 2" without mentioning the upper bound of 10 that the UI enforces.
✏️ Suggested wording
- 'hint.transformMaxRetries': '默认 2，最小 2；表示失败后最多重试的次数。',
+ 'hint.transformMaxRetries': '默认 2，范围 2-10；表示失败后最多重试的次数。',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/modules/i18n/locales/zh.mjs` around lines 491 - 492, Update the
`hint.transformMaxRetries` translation in the Chinese locale to mention the
enforced maximum of 10 alongside the existing default of 2 and minimum of 2;
leave the `field.transformMaxRetries` label unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web-ui/partials/index/panel-prompts.html (1)
8-10: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
tabindexmanagement for the ARIA tab pattern.The
role="tablist"/role="tab"/aria-selectedsemantics are correctly applied. However, the WAI-ARIA tab pattern recommends rovingtabindex— the active tab should havetabindex="0"and inactive tabstabindex="-1"— so that keyboard users tab into the tab list once and use arrow keys to switch. Currently both buttons are natively focusable and in the default tab order, which works but deviates from the standard pattern.If manual-activation-with-all-tabs-focusable is intentional, this can be skipped. Otherwise, consider adding
:tabindex="promptsSubTab === 'codex' ? 0 : -1"(and the inverse for the other button) plus arrow-key handling inswitchPromptsSubTab.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-ui/partials/index/panel-prompts.html` around lines 8 - 10, Implement roving tabindex for the prompts tablist: update both tab buttons in the prompts sub-tab markup so the active tab has tabindex 0 and the inactive tab has tabindex -1, based on promptsSubTab. Extend switchPromptsSubTab to support keyboard arrow navigation while preserving the existing click behavior and active-tab selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web-ui/partials/index/panel-prompts.html`:
- Around line 8-10: Implement roving tabindex for the prompts tablist: update
both tab buttons in the prompts sub-tab markup so the active tab has tabindex 0
and the inactive tab has tabindex -1, based on promptsSubTab. Extend
switchPromptsSubTab to support keyboard arrow navigation while preserving the
existing click behavior and active-tab selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7002537-7e96-4671-aa7d-b35c2456ef56
📒 Files selected for processing (3)
web-ui/partials/index/panel-prompts.htmlweb-ui/res/web-ui-render.precompiled.jsweb-ui/styles/modals-core.css
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🔇 Additional comments (2)
web-ui/res/web-ui-render.precompiled.js (1)
6659-6678: LGTM!web-ui/styles/modals-core.css (1)
696-727: LGTM!
2 participants
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests